src/js/msp.js (1)

402-413: ⚠️ Potential issue

_validateSendMessage ignores doCallbackOnError flag

Error-side invocation of callback_msp is unconditional. That breaks the original contract implied by the doCallbackOnError parameter and can trigger “double callbacks” or resolve a Promise that expected an actual response.

-_validateSendMessage(code, callback_msp) {
+_validateSendMessage(code, callback_msp, doCallbackOnError = false) {
     …
-    if (callback_msp) {
+    if (doCallbackOnError && callback_msp) {
         callback_msp();
     }

You’ll have to forward the flag from send_message.

src/js/msp.js (2)

423-431: Unused field – callbackOnError is never referenced

_createRequestObject stores callbackOnError, but no subsequent code reads it after the refactor. Either remove the property or reinstate the error-handling path.

Eliminating dead fields simplifies debugging and reduces cognitive load.

---

457-467: Aggressive timeout decay may under-shoot MIN_TIMEOUT

this.timeout-- happens on every send, regardless of frequency, and can push the value below MIN_TIMEOUT when timeout has already been clamped lower elsewhere.

-if (this.timeout > this.MIN_TIMEOUT) {
-    this.timeout--;
-}
+this.timeout = Math.max(this.MIN_TIMEOUT, this.timeout - 1);

A simple guard prevents negative or sub-minimum timeouts.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/js/msp.js (3)

390-398: Unused allocation for duplicates

requestObj is instantiated even when the call is considered a duplicate and then discarded. This happens on every duplicate invocation and can become a noticeable GC hot-spot under heavy traffic.

Move the allocation inside the if (!isDuplicateRequest) block so it is only created when needed.

---

445-456: Timer remains referenced after giving up – clear it to aid GC

When MAX_RETRIES is exceeded the function bails out but does not clearTimeout(requestObj.timer);.
While the timer has already fired, keeping the handle prevents some JS engines from reclaiming it promptly.

 if (requestObj.attempts >= this.MAX_RETRIES) {
     console.error(`MSP: Request ${requestObj.code} exceeded max retries (${this.MAX_RETRIES}), giving up`);
-    // Remove from callbacks to prevent memory leak
+    clearTimeout(requestObj.timer);          // 💡 explicit cleanup
+    // Remove from callbacks to prevent memory leak
     this._removeRequestFromCallbacks(requestObj);

---

411-413: Adaptive back-off step might starve high-latency links

this.timeout is decreased by a fixed 5 ms on every successful send. On slow links (e.g. Bluetooth) this can quickly shrink below the real RTT, leading to unnecessary retries. Consider decaying by a percentage (e.g. timeout *= 0.9) or pinning a lower bound based on recent RTT measurements instead of a constant 5 ms.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/js/msp.js (1)

399-420: ⚠️ Potential issue

Duplicate requests silently drop callbacks

When isDuplicateRequest is true, the new callback_msp is not stored anywhere, so callers issuing concurrent identical requests will never be notified.
This regresses previous behaviour and breaks APIs that rely on per-call completion.

Quick remedy: attach additional callbacks to the existing request object instead of discarding them.

-const isDuplicateRequest = this.callbacks.some((instance) => instance.requestKey === requestKey);
+const existingReq = this.callbacks.find((i) => i.requestKey === requestKey);
+const isDuplicateRequest = Boolean(existingReq);

…

-if (!isDuplicateRequest) {
+if (!isDuplicateRequest) {
     this._setupTimeout(requestObj, bufferOut);
     this.callbacks.push(requestObj);
+} else if (callback_msp) {
+    // fan-out: queue extra callback(s) on the first request
+    (existingReq.extraCallbacks ??= []).push(callback_msp);
 }

And, when the response arrives, invoke extraCallbacks as well.

src/js/msp.js (4)

376-389: Hash function may collide & yield negative keys

_createRequestKey uses a simple 32-bit rolling hash that can:

1. Produce the same hash for different payloads (higher collision risk on large traffic).
2. Return negative numbers, generating keys like 123:-842150451 which are harder to eyeball/debug.

Consider switching to a safer deterministic scheme, e.g. CRC-32 or base64 of the first 4 bytes plus length, to lower collision odds and keep keys human-readable.

---

421-428: Inefficient duplicate scan — O(N) each call

this.callbacks.some() does a linear search every time send_message is invoked.
With MAX_QUEUE_SIZE already tracked, switching to a Map<requestKey, requestObj> would give O(1) look-ups and simplify removal logic.

---

433-438: Timer handle lost if _setupTimeout is called twice

_setupTimeout overwrites requestObj.timer without clearing a possibly existing one.
Although current flow prevents re-entrance, guarding against future changes is cheap:

+if (requestObj.timer) {
+    clearTimeout(requestObj.timer);
+}
 requestObj.timer = setTimeout(() => {
     this._handleTimeout(requestObj, bufferOut);
 }, this.TIMEOUT);

---

439-472: Retry path never resets start — metrics become meaningless

requestObj.start is set once in send_message but never updated on retries, so any later duration calculations accumulate total wall-time, not per-attempt latency.

Either reset start inside _handleTimeout before re-arming the timer or drop it altogether if no longer used.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/js/msp.js (1)

482-491: _removeRequestFromCallbacks leaves Map/key tracking unresolved

If you adopt the Map optimisation above, ensure this method also deletes the requestKey entry to keep the structures in sync.

src/js/msp.js (4)

69-73: Expose retry parameters instead of hard-coding them

MAX_RETRIES, MAX_QUEUE_SIZE, and TIMEOUT are baked into the module, making it impossible to tune behaviour from the outside (e.g. per-board, per-environment, unit-tests). Consider reading them from CONFIGURATOR, a constructor argument, or at least exporting setters.

---

399-420: Duplicate check performs O(n) scans on every call

this.callbacks.some(...) linearly scans the queue. Under heavy traffic (e.g. telemetry floods) the cost grows quadratically.

Two easy wins:

1. Keep a Map<requestKey, requestObj> to get O(1) look-ups and removals.
2. Build bufferOut after the duplicate check to avoid unnecessary encoding work when the request is dropped.

---

433-438: Timer double-cleared – harmless but needless

_handleTimeout calls clearTimeout(requestObj.timer) (l.464) and _removeRequestFromCallbacks does the same. The second call is redundant and can be omitted to save a micro-op.

---

439-480: Constant timeout may hammer dead links

Retries fire every fixed TIMEOUT ms. On an unplugged device this means 10 exact re-transmits per second, wasting bandwidth and log space.
An exponential back-off (e.g. TIMEOUT * 2 ** attempts) curbs network noise and still converges quickly when the link is restored.

src/js/msp.js (1)

376-389: 🛠️ Refactor suggestion

_createRequestKey collision risk & signed-shift bug

The DJB-style hash on a Uint8Array will inevitably collide for different payloads and, because & 0xffffffff yields a signed 32-bit value, the string key can even become negative.
A wrong “duplicate” classification silently drops tracking / retries.

-        let hash = 0;
-        for (const byte of data) {
-            hash = ((hash << 5) - hash + byte) & 0xffffffff;
-        }
-        return `${code}:${hash}`;
+        // Use a stable unsigned hash (FNV-1a) to minimise collisions
+        let hash = 2166136261 >>> 0;
+        for (const byte of data) {
+            hash ^= byte;
+            hash = Math.imul(hash, 16777619) >>> 0;
+        }
+        return `${code}:${hash.toString(16)}`;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    // Helper function to create a unique key for request identification
    _createRequestKey(code, data) {
        if (!data || data.length === 0) {
            return `${code}:empty`;
        }

        // Use a stable unsigned hash (FNV-1a) to minimise collisions
        let hash = 2166136261 >>> 0;
        for (const byte of data) {
            hash ^= byte;
            hash = Math.imul(hash, 16777619) >>> 0;
        }

        return `${code}:${hash.toString(16)}`;
    },

🤖 Prompt for AI Agents

In src/js/msp.js around lines 376 to 389, the _createRequestKey function uses a
DJB-style hash that can produce collisions and negative values due to signed
32-bit shifts. To fix this, replace the hash calculation with a more robust
hashing method that avoids collisions and ensures the hash is always unsigned
and positive. For example, use a standard hashing library or implement a
non-signed hash function that produces a unique, non-negative key string for the
given data.

src/js/msp.js (4)

69-73: Freeze or namespace retry-config constants to prevent accidental mutation

Defining MAX_RETRIES, MAX_QUEUE_SIZE and TIMEOUT directly on the mutable MSP object means any consumer can overwrite them at runtime:

MSP.TIMEOUT = 1;   // 💥

If you really want them tweakable, expose an explicit setter.
Otherwise, lock them down:

-    MAX_RETRIES: 10,
-    MAX_QUEUE_SIZE: 50,
-    TIMEOUT: 1000,
+    MAX_RETRIES: 10,
+    MAX_QUEUE_SIZE: 50,
+    TIMEOUT: 1000,
 }
+
+Object.freeze(MSP);            // or freeze only MSP.retryConfig sub-object

---

376-389: Hashing scheme is fragile & collision-prone

_createRequestKey builds a 32-bit additive hash; for large or similar payloads collisions are easy, producing false “duplicates”.
Additionally hash may end up negative, so keys like 42:-123 and 42:327375 look different but are produced from different sessions of the same data length.

Consider:

• Use a stable digest (e.g. DJB2, MurmurHash) or crypto.subtle.digest('SHA-1', data) when available.
• Fall back to btoa(String.fromCharCode(...data)) for short payloads.
• At minimum, >>> 0 to keep the value unsigned.

---

421-428: Guard callback_sent with a functional check

promise() passes false, so callback_sent() may be a boolean.
A trivial typeof guard avoids accidental false is not a function mistakes if the signature changes upstream.

-if (sendInfo.bytesSent === bufferOut.byteLength && callback_sent) {
+if (sendInfo.bytesSent === bufferOut.byteLength && typeof callback_sent === "function") {
     callback_sent();
 }

---

479-489: O(N²) removal can hurt with large queues

indexOf + splice is fine for the 50-item cap, but if the limit is raised later it becomes quadratic.
Storing callbacks in a Map<requestKey, requestObj> would give O(1) insert/remove/lookup, simplify duplicate handling, and eliminate linear scans elsewhere.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/js/msp.js (2)

385-385: Duplicate detection by code only is too coarse; conflates distinct payloads and skews timers.

Using only instance.code misclassifies concurrent requests with the same code but different payloads. It also means only the first one gets a timer; later ones will sit untracked if the reply never comes.

Apply this minimal diff to defer duplicate detection until bufferOut exists and compare code + payload bytes:

-        const requestExists = this.callbacks.some((instance) => instance.code === code);
+        // Defer precise duplicate detection until after bufferOut is created
+        let requestExists = false;
+        let existing = null;

…and immediately after bufferOut is constructed (below Line 387), insert:

+        // Precise duplicate detection: same code AND identical payload
+        const outView = new Uint8Array(bufferOut);
+        existing = this.callbacks.find((i) => {
+            if (i.code !== code) return false;
+            const iv = new Uint8Array(i.requestBuffer);
+            if (iv.byteLength !== outView.byteLength) return false;
+            for (let k = 0; k < iv.byteLength; k++) {
+                if (iv[k] !== outView[k]) return false;
+            }
+            return true;
+        });
+        requestExists = Boolean(existing);

---

419-419: Chain duplicate MSP request callbacks instead of pushing new ones

To avoid queuing multiple identical MSP requests (which leads to redundant serial sends and un-timed callbacks), refactor send_message so that duplicate calls attach their callbacks to the first outstanding request, and update the response handler to fire those chained callbacks.

Key locations:

• src/js/msp.js (send_message, lines ~383–419): detect an existing request and only push a new entry when none exists; otherwise, append the new callback_msp to existing.chained.
• src/js/msp/MSPHelper.js (around line 1768): after firing the primary callback, invoke any chained callbacks before removing the entry.

Suggested diff for src/js/msp.js:

--- a/src/js/msp.js
+++ b/src/js/msp.js
@@ send_message(code, data, callback_sent, callback_msp, doCallbackOnError) {
-        const requestExists = this.callbacks.some((instance) => instance.code === code);
+        // Check for an in-flight request with the same code
+        const existing = this.callbacks.find((instance) => instance.code === code);
+        const requestExists = !!existing;
 
         const bufferOut = code <= 254
             ? this.encode_message_v1(code, data)
             : this.encode_message_v2(code, data);
 
         const obj = {
             code,
             requestBuffer: bufferOut,
             callback: callback_msp,
             callbackOnError: doCallbackOnError,
             start: performance.now(),
         };
 
-        if (!requestExists) {
-            obj.timer = setTimeout(...);
-        }
-
-        this.callbacks.push(obj);
+        if (!requestExists) {
+            // schedule timeout/retry only once
+            obj.timer = setTimeout(/* existing timeout handler */);
+            this.callbacks.push(obj);
+        } else if (callback_msp) {
+            // chain this callback onto the existing pending request
+            (existing.chained || (existing.chained = [])).push(callback_msp);
+        }
 
         // always send when there's payload or first request
         if (data || !requestExists) {
             serial.send(bufferOut, (sendInfo) => {
                 if (sendInfo.bytesSent === bufferOut.byteLength && callback_sent) {
                     callback_sent();
                 }
             });
         }

Suggested diff for src/js/msp/MSPHelper.js:

--- a/src/js/msp/MSPHelper.js
+++ b/src/js/msp/MSPHelper.js
@@ // inside the loop handling an MSP response (around line 1768)
             // fire primary callback
             if (!crcError || callbackOnError) {
-                if (callback) {
-                    callback({ command: code, data, length: data.byteLength, crcError });
-                }
+                if (callback) {
+                    callback({ command: code, data, length: data.byteLength, crcError });
+                }
+                // fire any chained callbacks for duplicate requests
+                const chained = dataHandler.callbacks[i].chained;
+                if (chained) {
+                    chained.forEach((cb) =>
+                        cb({ command: code, data, length: data.byteLength, crcError })
+                    );
+                }
             }

Once these changes are in place, the primary request will handle both timeout/retries and cleanup, while all additional callers simply attach their callback to the same in-flight request.

src/js/msp.js (5)

60-60: Fixed TIMEOUT constant: good step; consider making it configurable per-transport/test.

Hard-coding 1000ms is fine as a sane default. Consider allowing overrides via CONFIGURATOR or an optional param to send_message for test rigs and slow links.

---

378-383: Early-return invokes callback_msp() without error context.

Calling callback_msp() with no args conflates “not eligible to send” with “empty payload response.” Pass a sentinel (e.g., null or an Error-like object) so callers can discriminate.

-            if (callback_msp) {
-                callback_msp();
-            }
+            if (callback_msp) {
+                callback_msp({ error: "MSP send skipped: disconnected or virtual mode", code });
+            }

---

278-282: Leftover property set: messageIsJumboFrame is referenced but no longer tracked.

This looks like a remnant. It’s not defined in MSP anymore and resetting it every dispatch serves no purpose.

-        this.messageIsJumboFrame = false;

If you still need it, reintroduce a declared state property near Line 45.

---

67-67: Typo in comment (“charactor”).

-    cli_buffer: [], // buffer for CLI charactor output
+    cli_buffer: [], // buffer for CLI character output

---

1-465: Optional: add _removeRequestFromCallbacks helper to centralize timer cleanup.

Centralizes clearTimeout + splice to avoid dangling timers. Call it from the retry-cap branch and the normal completion path.

// Add inside MSP object (near callbacks_cleanup)
_removeRequestFromCallbacks(requestObj) {
    const idx = this.callbacks.indexOf(requestObj);
    if (idx > -1) {
        try { clearTimeout(this.callbacks[idx].timer); } catch (e) {}
        this.callbacks.splice(idx, 1);
    }
},

src/js/msp/debug/MSP_DEBUG_README.md (2)

317-330: Add a language hint to the fenced “File Structure” block to satisfy MD040.

-```
+```text
 src/js/msp/
 ├── MSPCodes.js
 ├── MSPConnector.js  
 ├── MSPHelper.js
 └── debug/
     ├── msp_queue_monitor.js     # Core monitoring functionality
     ├── msp_stress_test.js       # Stress testing framework
     ├── msp_debug_dashboard.js   # Visual dashboard UI
     ├── msp_test_runner.js       # Console command interface
     ├── msp_debug_tools.js       # Integration and auto-loading
     └── MSP_DEBUG_README.md      # Documentation

---

`1-365`: **Minor wording polish (optional).**

Static analysis flagged a lot of “There might be a mistake here” on headings because of emojis and bold markers. Content is clear; no action needed unless you want a perfectly clean linter run.

</blockquote></details>
<details>
<summary>src/js/msp/debug/index.js (1)</summary><blockquote>

`1-16`: **Barrel export looks good; watch for export name collisions.**

export * from './msp_debug_tools.js' will be a no-op if that file doesn’t export names (only side effects). Keep it if you plan to export from there later; otherwise you can drop it.

</blockquote></details>
<details>
<summary>src/js/msp/debug/msp_debug_tools.js (2)</summary><blockquote>

`36-52`: **Guard for non-browser/test environments and avoid double import.**

This file assumes window exists. In node/JSDOM tests that import it directly, window may be undefined. Also, you already import "./msp_queue_monitor.js" at the top; the dynamic import is redundant.

```diff
-if (window.location.hostname === "localhost" || window.location.hostname === "127.0.0.1") {
+if (typeof window !== "undefined"
+    && window.location
+    && (window.location.hostname === "localhost" || window.location.hostname === "127.0.0.1")) {
-    // Import the monitor and start it with minimal logging
-    import("./msp_queue_monitor.js").then(({ mspQueueMonitor }) => {
+    // Use the already-loaded export
+    import("./msp_queue_monitor.js").then(({ mspQueueMonitor }) => {
         mspQueueMonitor.addListener((status) => {
             // Only log alerts and significant events
             const alerts = Object.values(status.alerts).filter((a) => a);
             if (alerts.length > 0) {
                 console.warn("🚨 MSP Alert detected - check dashboard for details");
             }
         });
 
         mspQueueMonitor.startMonitoring(2000); // Monitor every 2 seconds
     });
 }

Optional: import { mspQueueMonitor } at the top instead of dynamic importing in dev.

---

12-34: Console banner in production (optional).

Consider gating the banner behind a dev flag to keep end-user consoles clean.

-console.log(`
+if (typeof window !== "undefined" && (window.location.hostname === "localhost" || window.location.hostname === "127.0.0.1")) console.log(`
 …
 `);

src/js/msp/debug/msp_debug_dashboard.js (2)

675-699: Harden update path against missing MSP/mspQueueMonitor during early startup.

If the dashboard is shown before MSP is initialized, mspQueueMonitor.getStatus() can throw. Wrap in try/catch and bail gracefully.

-        status = status || mspQueueMonitor.getStatus();
+        try {
+            status = status || mspQueueMonitor.getStatus();
+        } catch (e) {
+            console.warn("MSP Debug Dashboard: status unavailable yet", e);
+            return;
+        }

---

794-806: Avoid repeated DOM writes due to type mismatch in updateElement.

textContent is string; value can be number. Coerce once to string for a stable equality check.

-        if (element && element.textContent !== value) {
-            element.textContent = value;
+        if (element) {
+            const newText = String(value);
+            if (element.textContent !== newText) {
+                element.textContent = newText;
+            }
         }

src/js/msp/debug/msp_queue_monitor.js (2)

447-456: Timeout-rate deduction uses a possibly stale/undefined metrics.timeoutRate; compute locally

_calculatePerformanceGrade() should not rely on startMonitoring/_collectMetrics() having run. Compute the rate from raw counters to avoid undefined reads.

Apply this diff:

-        // Deduct for high timeout rate
-        if (this.metrics.timeoutRate > 0.1) {
+        // Deduct for high timeout rate
+        const timeoutRate =
+            this.metrics.totalRequests > 0 ? this.metrics.timeouts / this.metrics.totalRequests : 0;
+        if (timeoutRate > 0.1) {
             score -= 30;
-        } else if (this.metrics.timeoutRate > 0.05) {
+        } else if (timeoutRate > 0.05) {
             score -= 15;
         }

---

249-263: Expose immutable, plain snapshots to listeners to prevent accidental mutation of internal state

getStatus() shallow-copies metrics/alerts, but the Maps inside remain live references. Consider returning fully plain data (as in the report fix) or at least new Map instances to avoid accidental listener mutations.

Happy to provide a small _snapshot() helper that returns a deep-plain snapshot for both getStatus() and generateReport().

src/js/msp/debug/msp_test_runner.js (3)

43-53: Make periodic logging deterministic; avoid modulo-time race

if (Date.now() % 10000 < 500) can log 0–2 times depending on timer jitter. Track last-log time instead.

Apply this diff:

-            // Log every 10 seconds if monitoring
-            if (Date.now() % 10000 < 500) {
+            // Log every ~10 seconds if monitoring (deterministic)
+            const now = Date.now();
+            if (!this._lastQuickLogTs || now - this._lastQuickLogTs >= 10_000) {
+                this._lastQuickLogTs = now;
                 console.log(
                     `📊 MSP Status: Queue=${status.currentQueueSize}/${status.maxQueueSize}, Requests=${status.metrics.totalRequests}, AvgTime=${Math.round(status.metrics.avgResponseTime)}ms`,
                 );
             }

And initialize the tracking field:

 export const MSPTestRunner = {
     // Store the listener function so it can be removed later
     _quickMonitorListener: null,
+    _lastQuickLogTs: 0,

---

191-205: Preserve Map metrics in JSON download using a custom replacer

Even with the monitor fix, extra safety here ensures third-party objects serialize predictably.

Apply this diff:

-        const blob = new Blob([JSON.stringify(report, null, 2)], {
+        const replacer = (_key, value) => {
+            if (value instanceof Map) return Object.fromEntries(value);
+            return value;
+        };
+        const blob = new Blob([JSON.stringify(report, replacer, 2)], {
             type: "application/json",
         });

---

220-286: Quick health check: tighten timing consistency and clean up monitor even on early returns

Minor robustness: use performance.now() for latency consistency and ensure the monitor is stopped on all paths.

I can send a small patch converting the timing to performance.now() and using a single finally to stop monitoring.

src/js/msp/debug/msp_stress_test.js (1)

218-241: Duplicate detection relies on error message text; prefer a structured indicator

Counting duplicates via (err && err.message.includes("duplicate")) is brittle. If the MSP layer doesn’t expose a code, consider exposing a boolean/enum on rejection (e.g., { code: 'DUPLICATE' }) to avoid string matching.

If updating MSP isn’t feasible here, at least lower-case and guard to reduce flakiness:

-        const duplicateErrors = results.filter(
-            (r) => r.status === "rejected" || (r.value && r.value.error && r.value.error.includes("duplicate")),
-        ).length;
+        const duplicateErrors = results.filter((r) => {
+            if (r.status === "rejected") return true;
+            const msg = r.value && r.value.error;
+            return typeof msg === "string" && msg.toLowerCase().includes("duplicate");
+        }).length;

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/js/msp/debug/msp_queue_monitor.js (2)

270-284: Convert Maps to plain objects in status/report to make JSON exports accurate.

requestsByCode and errorsByType are Maps; JSON will serialise them as {}. This was previously flagged.

Apply this diff:

@@
-            metrics: { ...this.metrics },
+            metrics: this._toPlainMetrics(),
@@
-            rawMetrics: this.metrics,
+            rawMetrics: this._toPlainMetrics(),

Add this helper inside the class:

_toPlainMetrics() {
    const { requestsByCode, errorsByType, responseTimes, ...rest } = this.metrics;
    return {
        ...rest,
        responseTimes: Array.isArray(responseTimes) ? [...responseTimes] : [],
        requestsByCode: Object.fromEntries(requestsByCode),
        errorsByType: Object.fromEntries(errorsByType),
    };
}

Also applies to: 432-446

---

256-256: Guard all callbacks accesses to prevent runtime TypeError when MSP is uninitialised or torn down.

There are still direct .length/.map/iteration usages that can throw if this.msp.callbacks is undefined or not an array. Prior feedback already called this out.

Apply this diff:

@@
-        this.currentQueueSize = this.msp.callbacks.length;
+        this.currentQueueSize = this.msp.callbacks?.length ?? 0;
@@
-            currentQueueSize: this.msp.callbacks.length,
+            currentQueueSize: this.msp.callbacks?.length ?? 0,
@@
-            metrics: { ...this.metrics },
+            metrics: this._toPlainMetrics(),
@@
-            queueContents: this.msp.callbacks.map((req) => ({
+            queueContents: (this.msp.callbacks || []).map((req) => ({
                 code: req.code,
                 attempts: req.attempts || 0,
-                age: performance.now() - req.start,
+                age: req?.start ? performance.now() - req.start : 0,
                 hasTimer: !!req.timer,
             })),
@@
-        const callbacks = this.msp.callbacks;
+        const callbacks = this.msp.callbacks || [];

And inside the analysis loop:

@@
-            // Age analysis
-            const age = now - req.start;
+            // Age analysis (guard missing start)
+            const start = typeof req.start === "number" ? req.start : now;
+            const age = now - start;

Also applies to: 273-283, 290-352

src/js/msp/debug/msp_queue_monitor.js (8)

94-114: Make duplicate detection data-type safe (TypedArray/ArrayBuffer/Array) and simpler to read.

The current element-by-element loop assumes data has .length and index access. MSP payloads can be TypedArrays or ArrayBuffers. Centralise equality in a helper to avoid false negatives.

Apply this diff:

-        // Count potential duplicates already present in the queue (same code + same data)
+        // Count potential duplicates already present in the queue (same code + same data)
         try {
-            const callbacks = this.msp.callbacks || [];
-            const isDuplicate = callbacks.some((req) => {
-                if (req.code !== code) return false;
-                const a = data;
-                const b = req.data;
-                if (!a && !b) return true;
-                if (!a || !b) return false;
-                if (a === b) return true;
-                if (a.length !== b.length) return false;
-                for (let i = 0; i < a.length; i++) {
-                    if (a[i] !== b[i]) return false;
-                }
-                return true;
-            });
+            const callbacks = this.msp.callbacks || [];
+            const isDuplicate = callbacks.some((req) => req.code === code && this._dataEquals(data, req.data));
             if (isDuplicate) this.metrics.duplicates++;
         } catch (_) {
             // best-effort only
         }

Add this helper inside the class (e.g., near other private helpers):

// Type-tolerant binary/array equality
_dataEquals(a, b) {
    if (a === b) return true;
    if (!a || !b) return false;

    // ArrayBuffer or views
    const toU8 = (x) => {
        if (x instanceof ArrayBuffer) return new Uint8Array(x);
        if (ArrayBuffer.isView(x)) return new Uint8Array(x.buffer, x.byteOffset, x.byteLength);
        return null;
    };
    const au8 = toU8(a);
    const bu8 = toU8(b);
    if (au8 && bu8) {
        if (au8.byteLength !== bu8.byteLength) return false;
        for (let i = 0; i < au8.byteLength; i++) if (au8[i] !== bu8[i]) return false;
        return true;
    }

    // Arrays
    if (Array.isArray(a) && Array.isArray(b)) {
        if (a.length !== b.length) return false;
        for (let i = 0; i < a.length; i++) if (a[i] !== b[i]) return false;
        return true;
    }

    // Fallback for primitives/strings/objects
    try {
        return JSON.stringify(a) === JSON.stringify(b);
    } catch {
        return false;
    }
}

---

220-233: Emit an immediate snapshot on start to avoid a 1s blind spot.

Apply this diff:

         this.isMonitoring = true;
+        // capture a snapshot immediately
+        this._collectMetrics();
+        this._notifyListeners();
         this.monitoringInterval = setInterval(() => {
             this._collectMetrics();
             this._notifyListeners();
         }, intervalMs);

---

10-22: Stabilise the metrics shape by initialising successRate and timeoutRate to 0.

These keys are used elsewhere (e.g., grading) but are only populated by _collectMetrics(). Initialising prevents undefined reads before the first interval tick and after resetMetrics().

Apply this diff:

@@
         this.metrics = {
             totalRequests: 0,
             completedRequests: 0,
             failedRequests: 0,
             timeouts: 0,
             duplicates: 0,
             avgResponseTime: 0,
             maxResponseTime: 0,
             queuePeakSize: 0,
             requestsByCode: new Map(),
             responseTimes: [],
             errorsByType: new Map(),
+            successRate: 0,
+            timeoutRate: 0,
         };
@@
     resetMetrics() {
         this.metrics = {
             totalRequests: 0,
             completedRequests: 0,
             failedRequests: 0,
             timeouts: 0,
             duplicates: 0,
             avgResponseTime: 0,
             maxResponseTime: 0,
             queuePeakSize: 0,
             requestsByCode: new Map(),
             responseTimes: [],
             errorsByType: new Map(),
+            successRate: 0,
+            timeoutRate: 0,
         };

Also applies to: 389-401

---

468-499: Make grading independent of the timer tick by deriving rates locally.

Avoid relying on this.metrics.timeoutRate being refreshed by _collectMetrics().

Apply this diff:

     _calculatePerformanceGrade() {
         let score = 100;
 
-        // Deduct for high timeout rate
-        if (this.metrics.timeoutRate > 0.1) {
+        // Deduct for high timeout rate
+        const timeoutRate =
+            this.metrics.totalRequests > 0 ? this.metrics.timeouts / this.metrics.totalRequests : 0;
+        if (timeoutRate > 0.1) {
             score -= 30;
-        } else if (this.metrics.timeoutRate > 0.05) {
+        } else if (timeoutRate > 0.05) {
             score -= 15;
         }
@@
-        if (this.metrics.avgResponseTime > 2000) {
+        const avg = this.metrics.avgResponseTime || 0;
+        if (avg > 2000) {
             score -= 25;
-        } else if (this.metrics.avgResponseTime > 1000) {
+        } else if (avg > 1000) {
             score -= 10;
         }
@@
-        const failureRate =
-            this.metrics.totalRequests > 0 ? this.metrics.failedRequests / this.metrics.totalRequests : 0;
+        const failureRate =
+            this.metrics.totalRequests > 0 ? this.metrics.failedRequests / this.metrics.totalRequests : 0;

---

357-369: Return an unsubscribe from addListener for ergonomics.

Apply this diff:

     addListener(callback) {
-        this.listeners.push(callback);
+        this.listeners.push(callback);
+        return () => this.removeListener(callback);
     }

---

696-698: Avoid instantiating just to destroy; guard the proxy destroy().

Calling mspQueueMonitor.destroy() when no instance exists will currently attempt to construct and then destroy. Early-return instead.

Apply this diff:

-    destroy(...args) {
-        return this.instance.destroy(...args);
-    },
+    destroy(...args) {
+        if (!_mspQueueMonitorInstance) return;
+        return _mspQueueMonitorInstance.destroy(...args);
+    },

---

179-215: Rename “memory leak” alert or refine detection to avoid false positives.

Queue length spikes indicate backlog, not necessarily leaks. Either rename to “backlog” or track monotonic growth over a window (e.g., > N samples increasing).

If you’d like, I can add a rolling-window slope detector and rename the flag to backlog while keeping a separate memoryLeakSuspected derived from >X minutes of continuous growth.

---

217-233: Minor: rate/average recomputation could be incremental and cheaper.

The rolling average currently reduces the entire window on each completion. For a 100-sample cap this is fine; if you increase it later, consider keeping a running sum.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/js/msp/debug/msp_stress_test.js (2)

666-673: Fix: runSpecificTest hardcodes "PASSED"; derive status from the actual result

This still reports "PASSED" regardless of failures returned by the test method. Reuse the status derivation helper.

Apply this diff:

         try {
             const result = await testMethod();
             return {
                 name: testName,
-                status: "PASSED",
+                status: this.getTestStatus(testName, result),
                 result,
                 metrics: this.monitor.getStatus(),
             };

---

393-442: Connection Disruption Test Must Toggle Module-Level Serial Instance

The stress-test’s testConnectionDisruption currently flips this.msp.serial.connected, but msp.send_message()/promise() checks the singleton serial imported in msp.js (import { serial } from "./serial.js"). Mutating this.msp.serial has no effect on the branch in send_message, so the “disconnected” path is never exercised.

• File: src/js/msp/debug/msp_stress_test.js
• Method: testConnectionDisruption

Apply these changes:

1. Import the real serial singleton from serial.js
2. Replace all this.msp.serial references with moduleSerial

--- a/src/js/msp/debug/msp_stress_test.js
+++ b/src/js/msp/debug/msp_stress_test.js
@@ 1,5c1,7
-import { mspQueueMonitor } from "./msp_queue_monitor.js";
+import { mspQueueMonitor } from "./msp_queue_monitor.js";
+// Use the same serial instance that MSP methods reference
+import { serial as moduleSerial } from "../../serial.js";

 export class MSPStressTest {
@@ 393,7c395,7
-        const originalConnected = this.msp.serial?.connected;
+        const originalConnected = moduleSerial.connected;
 
         try {
-            if (this.msp.serial) {
-                this.msp.serial.connected = false;
+            moduleSerial.connected = false;
             }
 
             // Try to send requests while "disconnected"
@@ 414,7c416,7
-            if (this.msp.serial) {
-                this.msp.serial.connected = originalConnected;
+            moduleSerial.connected = originalConnected;
             }
 
             // Test recovery
@@ 422,7c424,7
-        } finally {
-            // Ensure connection is restored
-            if (this.msp.serial) {
-                this.msp.serial.connected = originalConnected;
+        } finally {
+            // Ensure connection is restored even on error
+            moduleSerial.connected = originalConnected;
         }

This ensures you’re toggling the exact serial.connected flag that send_message checks.

src/js/msp/debug/msp_stress_test.js (4)

248-260: Duplicate test metrics: distinguish duplicate rejections from other failures

Counting every rejection as a "duplicate" can mask unrelated errors. Separate them so the status helper can be precise.

-        const duplicateErrors = results.filter(
-            (r) => r.status === "rejected" || (r.value && r.value.error && r.value.error.includes("duplicate")),
-        ).length;
+        const isDupErr = (val) => typeof val?.error === "string" && /duplicat(e|ion)/i.test(val.error);
+        const duplicateErrors = results.filter(
+            (r) => r.status === "rejected" || isDupErr(r.value)
+        ).length;
+        const otherFailures = results.filter(
+            (r) => r.status === "rejected" || (!!r.value && !!r.value.error)
+        ).length - duplicateErrors;
 
         return {
             duplicatesSent: duplicateCount,
             successful,
             duplicateRejections: duplicateErrors,
+            otherFailures,
             queueSizeAfter: this.msp.callbacks.length,
         };

---

268-299: Timeout Recovery: verify the error is actually a timeout before claiming success

Any rejection is treated as a timeout. Tighten detection to avoid false positives and improve diagnostics.

-            try {
-                await this.msp.promise(code, null);
-                return { error: "Expected timeout but request succeeded" };
-            } catch (error) {
+            try {
+                await this.msp.promise(code, null);
+                return { error: "Expected timeout but request succeeded" };
+            } catch (error) {
                 const timeoutTime = performance.now() - startTime;
 
                 // Test that new requests work after timeout
                 this.msp.TIMEOUT = originalTimeout;
                 await this.wait(200);
 
                 const recoveryStart = performance.now();
                 await this.msp.promise(this.testCodes.MSP_IDENT, null);
                 const recoveryTime = performance.now() - recoveryStart;
 
                 return {
-                    timeoutOccurred: true,
+                    timeoutOccurred: /timeout/i.test(error?.message || "") || error?.name === "TimeoutError" || error?.code === "ETIMEDOUT",
                     timeoutDuration: timeoutTime,
                     recoveryTime,
                     queueCleanedUp: this.msp.callbacks.length === 0,
                 };
             }

Optionally add a helper to centralize timeout detection if used elsewhere.

---

367-388: Queue Overflow: count overflow-specific rejections, not all errors

Right now any rejection increments overflow count, which can conflate unrelated failures. Filter by recognizable overflow messages to reduce false positives.

-        const results = await Promise.allSettled(promises);
-        const rejected = results.filter((r) => r.status === "rejected" || r.value?.error).length;
+        const results = await Promise.allSettled(promises);
+        const isOverflow = (val) =>
+            typeof val?.error === "string" && /(overflow|queue\s*full)/i.test(val.error);
+        const rejectedDueToOverflow = results.filter(
+            (r) => r.status === "rejected" || isOverflow(r.value)
+        ).length;
 
         return {
             attemptedRequests: overflowCount,
             maxQueueSize: maxQueue,
-            rejectedDueToOverflow: rejected,
-            overflowHandled: rejected > 0,
+            rejectedDueToOverflow,
+            overflowHandled: rejectedDueToOverflow > 0,
             finalQueueSize: this.msp.callbacks.length,
         };

---

545-567: Report: include a guard for empty results to avoid division-by-zero and NaN successRate

Not likely here, but runSpecificTest consumers might call generateTestReport([]). Cheap guard improves resilience.

-        const totalTests = results.length;
+        const totalTests = results.length || 0;
         const passedTests = results.filter((r) => r.status === "PASSED").length;
-        const failedTests = totalTests - passedTests;
+        const failedTests = Math.max(0, totalTests - passedTests);
@@
-            successRate: passedTests / totalTests,
+            successRate: totalTests ? passedTests / totalTests : 0,

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.